Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing join mapping - take 1 #410

Merged
merged 7 commits into from Mar 20, 2018
Merged

Conversation

rafiki270
Copy link
Member

@rafiki270 rafiki270 commented Mar 20, 2018

I had to pass the info about the model down the generics chain ...

In the firstValue method I now first try to check for the "main" id and then have a fallback onto any "foreign" keys to the model. Obvious downside to this is that you won't be able to get a join field with the same name as you may have in the "main" table but its still an improvement me thinks ...

Sorry for the whitespaces, stupid copy/paste from my projects dependencies ... will remove if you confirm I am on the right path ...

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely on the right track, but have some ideas on how to make it even better. Thanks!

@@ -5,26 +5,26 @@ import Foundation
public final class QueryBuilder<Model, Result> where Model: Fluent.Model, Model.Database: QuerySupporting {
/// The query being built.
public var query: DatabaseQuery<Model.Database>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an xcode option to delete whitespace on empty lines. please enable that to reduce the git noise here :)

let q = self.query
let resultTransformer = self.resultTransformer
return connection.flatMap(to: Void.self) { conn in
return Model.Database.execute(query: q, into: { row, conn in
resultTransformer(row, conn).map(to: Void.self) { result in
return try handler(result)
}.catch { error in
print("[Fluent] Skipping row: \(error)")
}.catch { error in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting needs fixing here

@@ -43,21 +43,33 @@ extension QueryField: ExpressibleByStringLiteral {

extension Dictionary where Key == QueryField {
/// Accesses the _first_ value from this dictionary with a matching field name.
public func firstValue(forField fieldName: String) -> Value? {
public func firstValue(forField fieldName: String, on entity: String) -> Value? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this "firstValue" should stay the same (no on entity label). Use value(forEntity entity: String, atField field: String) instead. A QueryField with same table name and entity is the same query field, so it doesn't make sense to say "first" value for a specific field name and entity--it's the "only value"

return try D.init(from: decoder)
}
}

/// MARK: Private

fileprivate final class _QueryDataDecoder<Database>: Decoder where Database: QuerySupporting {
fileprivate final class _QueryDataDecoder<M>: Decoder where M: Model, M.Database: QuerySupporting {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to keep this generic on the Database and just pass in entity: String during init so that this can be used for non-model types (very common) as well!

@rafiki270 rafiki270 force-pushed the fixing-join-mapping branch 2 times, most recently from 1801910 to dd6ebf6 Compare March 20, 2018 21:10
@rafiki270 rafiki270 force-pushed the fixing-join-mapping branch 2 times, most recently from 97be58b to 8e7688a Compare March 20, 2018 21:49
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is perfect, thank you @rafiki270!

@tanner0101 tanner0101 added this to the 3.0.0-rc.2 milestone Mar 20, 2018
@tanner0101 tanner0101 added the enhancement New feature or request label Mar 20, 2018
@tanner0101 tanner0101 self-assigned this Mar 20, 2018
@tanner0101 tanner0101 changed the base branch from nio to master March 20, 2018 22:10
@rafiki270
Copy link
Member Author

@Tanner now it should be good to go, I had to test in my app first to find all still works

@tanner0101 tanner0101 merged commit 666352d into vapor:master Mar 20, 2018
@rafiki270 rafiki270 deleted the fixing-join-mapping branch March 20, 2018 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Vapor 3
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

2 participants